Bind trait props/consts before parent class #15878
Conversation
| trait TestTrait2 { | ||
| public final const Constant = 123; | ||
| } | ||
|
|
||
| class BaseClass2 { | ||
| public final const Constant = 456; | ||
| } | ||
|
|
||
| class DerivedClass2 extends BaseClass2 { | ||
| use TestTrait2; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| Fatal error: BaseClass2 and TestTrait2 define the same constant (Constant) in the composition of DerivedClass2. However, the definition differs and is considered incompatible. Class was composed in %s on line %d | ||
| Fatal error: DerivedClass1::Constant cannot override final constant BaseClass1::Constant in %s on line %d |
There was a problem hiding this comment.
This is a behavior change, that may break some PHP code.
There was a problem hiding this comment.
Right. This is (IMO) clearly a bug, but it may be better to commit to master anyway.
| @@ -0,0 +1,30 @@ | |||
| --TEST-- | |||
| GH-15753: Overriding readonly properties from traits don't allow assignment from the child | |||
There was a problem hiding this comment.
The test works fine on unpatched master, but fails on PHP-8.2. Is this a backport?
There was a problem hiding this comment.
No, this works on master because of the changes to readonly in https://wiki.php.net/rfc/asymmetric-visibility-v2#relationship_with_readonly. Namely, readonly properties can, by default, now be initialized from all sub-classes. For master, we should add a better test.
|
A couple of weeks ago I would have suggested to apply this master only, but now I'm unsure whether it should still go into PHP-8.4. Maybe the @php/release-managers-84 want to have a look. |
|
I'm also unsure about this myself. This late and this ambiguous, I think it'd be better off in 8.5. |
|
Can you explain what you mean by ambiguous? It might be good to adjust methods too, which should make behavior consistent and predictable. But it might require a bit more changes. |
|
I was referring to Dmitry's comment about possible fallout. |
|
Ok, thanks for the clarification. I will make the changes for methods, and then target this to master. |
This more accurately matches the "copy & paste" semantics described in the documentation. The same could be done for methods. However, abstract trait methods diverge from this behavior, given that a parent method can satisfy trait methods used in the child. In that case, the method is not copied, but the check must be performed after the parent has been bound. For now, just bother with properties and constants. Fixes phpGH-15753
* Don't iterate trait methods if they don't contain an abstract method * Don't build the exclude and alias table twice
This more accurately matches the "copy & paste" semantics described in the documentation. The same could be done for methods. However, abstract trait methods diverge from this behavior, given that a parent method can satisfy trait methods used in the child. In that case, the method is not copied, but the check must be performed after the parent has been bound. For now, just bother with properties and constants.
Fixes GH-15753
I'm unsure if this should be backported. It might be safer to just commit for master.